[ty] No boundness analysis for implicit instance attributes#20128
[ty] No boundness analysis for implicit instance attributes#20128
Conversation
Diagnostic diff on typing conformance testsNo changes detected when running ty on typing conformance tests ✅ |
|
62b81ec to
33f6df6
Compare
…20133) ## Summary Add regression benchmarks for the problematic cases in astral-sh/ty#758. I'd like to merge this before #20128 to measure the impact (local tests show that this will "solve" both cases).
33f6df6 to
ba3c7e8
Compare
| } | ||
|
|
||
| fn analyze_single(db: &dyn Db, predicate: &Predicate) -> Truthiness { | ||
| let _span = tracing::trace_span!("analyze_single", ?predicate).entered(); |
There was a problem hiding this comment.
I have added and removed this so many times... it's obviously useful, so I guess there's no harm in leaving it.
ba3c7e8 to
ceab895
Compare
CodSpeed Instrumentation Performance ReportMerging #20128 will improve performances by ×3.9Comparing Summary
Benchmarks breakdown
|
AlexWaygood
left a comment
There was a problem hiding this comment.
Branch-dependent attributes are much more common in stub files than in runtime code. I wonder if it would make sense to add a diagnostic that enforces that you should never have any implicit instance attributes in a stub file. I.e., since we will now have different behaviour for the two different snippets, we could add a diagnostic that says that in a stub file you should never do this:
import sys
class F:
def __init__(self) -> None:
if sys.version_info >= (3, 13):
self.x: intand that instead you should always do this:
import sys
class Foo:
if sys.version_info >= (3, 13):
x: int(but the new diagnostic could probably trigger for any implicit instance attribute in a stub file, not just ones inside branches -- it'll be simpler to implement that way, and there's no real reason to ever have any implicit instance attributes in a stub file)
Then again, now that I think about it, that's all already covered by https://docs.astral.sh/ruff/rules/non-empty-stub-body/, so maybe we should just recommend that users should enable that Ruff rule on their stub files
I don't see any reason why someone would declare the type of instance attributes the complicated way in a stub, if there is a much easier way to just declare it on the body of the class. The implicit syntax wouldn't enable any use cases that wouldn't be covered by a simple |
there's no reason, no! But, folks do strange things sometimes -- I'd wager money that there's a stub file out there somewhere that does something like that 😆 |
* main: Fix mdtest ignore python code blocks (#20139) [ty] add support for cyclic legacy generic protocols (#20125) [ty] add cycle detection for find_legacy_typevars (#20124) Use new diff rendering format in tests (#20101) [ty] Fix 'too many cycle iterations' for unions of literals (#20137) [ty] No boundness analysis for implicit instance attributes (#20128) Bump 0.12.11 (#20136) [ty] Benchmarks for problematic implicit instance attributes cases (#20133) [`pyflakes`] Fix `allowed-unused-imports` matching for top-level modules (`F401`) (#20115) Move GitLab output rendering to `ruff_db` (#20117) [ty] Evaluate reachability of non-definitely-bound to Ambiguous (#19579) [ty] Introduce a representation for the top/bottom materialization of an invariant generic (#20076) [`flake8-async`] Implement `blocking-http-call-httpx` (`ASYNC212`) (#20091) [ty] print diagnostics with fully qualified name to disambiguate some cases (#19850) [`ruff`] Preserve relative whitespace in multi-line expressions (`RUF033`) (#19647)
…stral-sh#20133) ## Summary Add regression benchmarks for the problematic cases in astral-sh/ty#758. I'd like to merge this before astral-sh#20128 to measure the impact (local tests show that this will "solve" both cases).
…h#20128) ## Summary With this PR, we stop performing boundness analysis for implicit instance attributes: ```py class C: def __init__(self): if False: self.x = 1 C().x # would previously show an error, with this PR we pretend the attribute exists ``` This PR is potentially just a temporary measure until we find a better fix. But I have already invested a lot of time trying to find the root cause of astral-sh/ty#758 (and [this example](astral-sh/ty#758 (comment)), which I'm not entirely sure is related) and I still don't understand what is going on. This PR fixes the performance problems in both of these problems (in a rather crude way). The impact of the proposed change on the ecosystem is small, and the three new diagnostics are arguably true positives (previously hidden because we considered the code unreachable, based on e.g. `assert`ions that depended on implicit instance attributes). So this seems like a reasonable fix for now. Note that we still support cases like these: ```py class D: if False: # or any other expression that statically evaluates to `False` x: int = 1 D().x # still an error class E: if False: # or any other expression that statically evaluates to `False` def f(self): self.x = 1 E().x # still an error ``` closes astral-sh/ty#758 ## Test Plan Updated tests, benchmark results
## Summary This is a simpler approach to the performance issues mentioned in #23520. isort's profiling results suggested that #22794 added a slow-converging calculation rather than adding a specific hotspot to the type inferer. What makes type inference for loop variables more troublesome than other types of type inference is the calculation of reachability. For other types, such as implicit attribute type inference, reachability analysis is not performed for each attribute binding (reverted due to performance issues: #20128, astral-sh/ty#2117). They are all treated as reachable. Loop variables perform this heavy calculation (omitting reachability analysis from the `LoopHeader` branch of `infer_loop_header_definition` and `place_from_bindings_impl` will significantly improve performance). It appears that slow convergence for one variable in a loop block will also slow down the inference of all other definitions in the block that depend on it. To alleviate the issue, this PR reduces the value of `MAX_RECURSIVE_UNION_LITERALS` from 10 to 5. This will result in faster convergence when the loop variable grows like `Literal[0, 1, ...]`. Local measurements show that this PR alone improved the isort inspection time by about 37%. I chose 5 as the new value because I felt it offered a good balance between type inference precision and performance, based on the following measurement results: | Threshold | Mean | vs 10 | |-----------|--------|---------| | 4 | 0.89s | -38% | | 5 | 0.91s | -37% | | 6 | 1.05s | -27% | | 7 | 1.06s | -27% | | 8 | 1.23s | -14% | | 9 | 1.26s | -13% | | 10 (current) | 1.43s | — | It has been observed that this PR and another mitigation, #23520, are compatible, resulting in a total performance recovery of about 40-50%. ## Test Plan N/A
Summary
With this PR, we stop performing boundness analysis for implicit instance attributes:
This PR is potentially just a temporary measure until we find a better fix. But I have already invested a lot of time trying to find the root cause of astral-sh/ty#758 (and this example, which I'm not entirely sure is related) and I still don't understand what is going on. This PR fixes the performance problems in both of these problems (in a rather crude way).
The impact of the proposed change on the ecosystem is small, and the three new diagnostics are arguably true positives (previously hidden because we considered the code unreachable, based on e.g.
assertions that depended on implicit instance attributes). So this seems like a reasonable fix for now.Note that we still support cases like these:
closes astral-sh/ty#758
Test Plan
Updated tests, benchmark results